Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Part 3 of Restructuring BeaconBlock Proto - Remove Special Record #1067

Merged
merged 6 commits into from
Dec 10, 2018

Conversation

terencechain
Copy link
Member

Part of #781

This PR removes special record from beacon block

@terencechain terencechain self-assigned this Dec 10, 2018
Copy link
Contributor

@rauljordan rauljordan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's also use ranges here for proto fields to be safe

// Specials consist of exist, penalities, etc.
repeated SpecialRecord specials = 7 [deprecated=true]; // Deprecated in favor of unify specials object w/ attestations.
repeated bytes proposer_signature = 8; // Type of [uint384]?
repeated bytes ancestor_hash32s = 2;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a typo?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think 32s implies there is a multiple of ancestor hashes because repeated

Copy link
Member

@prestonvanloon prestonvanloon Dec 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No typo here. Hash32s is plural


// Block Body
BeaconBlockBody body =79;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spacing

@codecov
Copy link

codecov bot commented Dec 10, 2018

Codecov Report

Merging #1067 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1067      +/-   ##
==========================================
- Coverage   71.19%   71.19%   -0.01%     
==========================================
  Files          76       76              
  Lines        4576     4575       -1     
==========================================
- Hits         3258     3257       -1     
  Misses       1011     1011              
  Partials      307      307

// Specials consist of exist, penalities, etc.
repeated SpecialRecord specials = 7 [deprecated=true]; // Deprecated in favor of unify specials object w/ attestations.
repeated bytes proposer_signature = 8; // Type of [uint384]?
repeated bytes ancestor_hash32s = 2;
Copy link
Member

@prestonvanloon prestonvanloon Dec 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No typo here. Hash32s is plural

// Block Body
BeaconBlockBody body = 9;
BeaconBlockBody body = 1000;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why 1000? Do we expect a lot of fields in this message?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not.. I'll change it back to 9

repeated SpecialRecord specials = 7 [deprecated=true]; // Deprecated in favor of unify specials object w/ attestations.
repeated bytes signature = 8; // Type of [uint384]?
repeated bytes ancestor_hash32s = 2;
bytes state_root_hash32 = 3;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, once we launch a v1, we can never change the IDs. It's OK for now though

I'll be sure to document this, but read this for your FYI.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Reading it now

@terencechain terencechain merged commit 3c7b5bb into master Dec 10, 2018
@nisdas nisdas deleted the remove-special branch January 28, 2019 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants